Handle missing scheme in animated image rule #545
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…date the url is existing
…nly instead of duplicated inside other functions
…ror_when_image_lacks_scheme
pattonwebz
commented
Mar 25, 2024
includes/helper-functions.php
Outdated
Comment on lines
917
to
923
| $file = edac_url_add_scheme_if_not_existing( $filename ); | ||
| $url_exists = edac_url_exists( $file ); | ||
| // if the https version does not exist, try http. | ||
| if ( false === $url_exists ) { | ||
| $file = edac_url_add_scheme_if_not_existing( $filename, 'http' ); | ||
| $url_exists = edac_url_exists( $file ); | ||
| } |
There was a problem hiding this comment.
I thought about this over the weekend and I want to just do away with the nested if logic here and instead assume it will always have only what the site protocal is since the page it loads on would dictate it. Thoughts on dropping the internal if here that falls back to http?
There was a problem hiding this comment.
Yes, we can remove this logic check.
SteveJonesDev
requested changes
Mar 26, 2024
includes/helper-functions.php
Outdated
| * @return string The filename unchanged if it doesn't look like a URL, or with a scheme added if it does. | ||
| */ | ||
| function edac_url_add_scheme_if_not_existing( string $file, string $site_protocol = 'https' ): string { | ||
| if ( str_starts_with( $file, $site_protocol ) || ! str_starts_with( $file, '//' ) ) { |
There was a problem hiding this comment.
Should we check if a protocol doesn't exist at all? Right now a URL like this isn't caught.
<img src="assets.rewardstyle.com/images/search/350.gif" />The capability is supporting urls that are neither relative not protocol-relative. It's an edge case for sure but no harm in adding support for it.
…tive urls as well Also adds an explicit list of valid schemes as well to support more edge cases
SteveJonesDev
requested changes
Mar 26, 2024
There was a problem hiding this comment.
URL's like this assets.rewardstyle.com/images/search/350.gif are still being opened without a protocol.
Here is the warning:
[26-Mar-2024 16:39:23 UTC] PHP Warning: fopen(rewardstyle.com/images/search/350.gif): Failed to open stream: No such file or directory in /Users/stevejones/Documents/Repositories/accessibility-checker/includes/helper-functions.php on line 950
…obust Thanks to the newly added tests for catching my last change here as being a change that had downstream effects :)
…obust Thanks to the newly added tests for catching my last change here as being a change that had downstream effects :)
…causes_error_when_image_lacks_scheme' into william/544/animage_image_check_causes_error_when_image_lacks_scheme # Conflicts: # includes/helper-functions.php
pattonwebz
deleted the
william/544/animage_image_check_causes_error_when_image_lacks_scheme
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The rule that handles detection of animated gifs and webp images was causing an error on sites when the image src was missing a scheme. This PR solves for that by creating a helper function to add a scheme and verify the URL is accessible before opening the file to analyse it's frames.
Fixes: #544